Move LSTGeometry repo into ESProducer and standalone binary #203

ariostas · 2025-09-26T15:32:52Z

This is still a work in progress, but I'm opening a draft PR so you can track the progress.

This is relatively close to working for standalone, but I still have to figure out the ESProducer part.

slava77 · 2025-09-26T22:41:10Z

I expect that the final code will not use a csv file to generate the geometry bin/txt file.
RecoTracker/MkFit/plugins/MkFitGeometryESProducer.cc and the standalone representation will be derived similar to RecoTracker/MkFit/test/DumpMkFitGeometry.cc
... at least as an inspiration

ariostas · 2025-10-24T20:37:41Z

RecoTracker/LSTCore/interface/LSTGeometry/ModuleMapMethods.h

+          std::vector<Polygon> difference;
+          for (auto &ref_polygon_piece : ref_polygon) {
+              std::vector<Polygon> tmp_difference;
+              boost::geometry::difference(ref_polygon_piece, tar_polygon, tmp_difference);


@slava77 currently the bottleneck is with polygon operations like this one. I'm using boost::geometry, but maybe you have a suggestion for another library. Currently, I'm having some issues with the ordering of points ending up creating self-intersecting polygons or negative areas.

Is this really the slowest part? I would think that the straight line tracing should be the fastest.

is it going to be faster to add all polygons from list_of_detids_etaphi_layer_tar and then subtract them from ref_detid polygon?

here when considering new_tar_detids_to_be_considered and in the tar_detids_to_be_considered loop I'd expect that it would be much faster to place detids in eta-phi bins in each layer with some coarse binning (perhaps 32x32 or so) and then loop only over these eta-phi bins instead over all detids in a layer (access det_geom.getEndcapLayerDetIds(layer, etaphiBin) )

For the endcap, are the layers here for the positive and negative endcaps different or the same? If the same, then it sounds like an obvious speedup should be to skip the wrong side. The eta-phi binning will address this automatically

I didn't mean this point in particular, just that the polygone operations through the code take up most of the time.

Yeah, binning sounds like the way to go. And for the endcap, I think the layer includes both, so that was another wasteful thing.

slava77 · 2025-10-24T22:15:38Z

RecoTracker/LSTCore/interface/LSTGeometry/LSTMath.h

+      double xnew = x * std::cos(-refphi) - y * std::sin(-refphi);
+      double ynew = x * std::sin(-refphi) + y * std::cos(-refphi);
+      x = xnew;
+      y = ynew;
+    }
+    double phi = std::atan2(y, x);
+    double eta = std::copysign(-std::log(std::tan(std::atan(std::sqrt(x * x + y * y) / std::abs(z)) / 2.)), z);


I'm not sure why this sincos rotation to xnew,ynew is done.
why is the phi here not simply atan2(y,x)-refphi ?

It would be even faster if phi for all corners is precomputed.

Also, eta doesn't depend on refphi, perhaps it's easier to precompute eta for all the corners and use the values directly (including the zshifts of 0, ±10).

I'm not sure, I took it directly from here, but I'll think about it.

https://github.com/SegmentLinking/LSTGeometry/blob/15f482747748e89632fbef65989355ddbb8d94d7/LSTMath.py#L150-L159

And yeah, pre-computing things would be good

slava77 · 2025-10-24T22:17:55Z

RecoTracker/LSTCore/interface/LSTGeometry/ModuleMapMethods.h

+        std::vector<Polygon> ref_polygon;
+        ref_polygon.push_back(getEtaPhiPolygon(det_geom.getCorners(ref_detid), refphi, zshift));
+
+        // Check whether there is still significant non-zero area


it would help to add some more detailed description of what is done (it took me a while to stare at this code to figure out what's happening)

I added a bit of a longer explanation above. Let me know if that's sufficient

slava77 · 2025-10-25T02:04:04Z

RecoTracker/LSTCore/interface/LSTGeometry/ModuleMapMethods.h

+
+      for (int zshift : {0, 10, -10}) {
+        std::vector<Polygon> ref_polygon;
+        ref_polygon.push_back(getEtaPhiPolygon(det_geom.getCorners(ref_detid), refphi, zshift));


We probably don't really need polygons (and boost). I'd treat modules as rectangles using the larger side (xyz rectangles are wider in phi on the lower r side) for the positive overlap tests and the narrower side for the subtractive overlaps.

The simplest subtractive overlap can be done by breaking a module in e.g. 10x10 bins and just brute force checking each bin if it's covered or not to get to the endcap modules not covered by barrel.
A more precise and perhaps faster is to collect (ordered) etas and phis from the target modules and bin the reference module in rectangles along these etas and phis.

Yeah, it would be ideal to ditch polygons and just work with a simplified approximation. I'll work on this.

ariostas · 2025-11-03T20:26:58Z

After binning, it takes about 15 seconds, which is starting to be reasonable. There are more improvements that can be made to make it faster, but for now I'll switch gears to get the CMSSW part in place.

ariostas · 2025-12-03T17:26:59Z

Not sure if it will work, but let's see what happens.

/run cmssw

github-actions · 2025-12-03T18:24:54Z

There was a problem while building and running with CMSSW. The logs can be found here.

github-actions · 2025-12-03T19:13:33Z

There was a problem while building and running with CMSSW. The logs can be found here.

ariostas · 2026-01-06T19:10:37Z

/run cmssw

github-actions · 2026-01-06T19:53:08Z

There was a problem while building and running with CMSSW. The logs can be found here.

github-actions · 2026-01-06T21:45:05Z

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks

The full set of validation and comparison plots can be found here.

github-actions · 2026-01-06T21:52:48Z

There was a problem while building and running with CMSSW. The logs can be found here.

ariostas · 2026-01-06T21:56:39Z

Hmm I'll have to make some tweaks to be able to test the low pt setup, so I'll wrap this up tomorrow.

github-actions · 2026-01-07T15:47:05Z

There was a problem while building and running with CMSSW. The logs can be found here.

github-actions · 2026-01-07T18:42:22Z

There was a problem while building and running with CMSSW. The logs can be found here.

github-actions · 2026-01-07T20:04:13Z

There was a problem while building and running with CMSSW. The logs can be found here.

ariostas · 2026-01-08T15:32:55Z

/run gpu-cmssw lowpt

github-actions · 2026-01-08T16:47:19Z

The PR was built and ran successfully with CMSSW (low pT setup) on GPU. Here are some plots.

OOTB All Tracks

The full set of validation and comparison plots can be found here.

ariostas · 2026-01-08T17:09:20Z

@slava77 I don't think the lowpt setup is working properly. The plots look the same, so I'm wondering if it was even working before. These are the extra lines that the CI adds to the .py file: https://github.com/SegmentLinking/TrackLooper-actions/blob/cmssw/lowpt_mod.py . Do you know what might be missing?

slava77 · 2026-01-08T17:42:53Z

@slava77 I don't think the lowpt setup is working properly. The plots look the same, so I'm wondering if it was even working before. These are the extra lines that the CI adds to the .py file: https://github.com/SegmentLinking/TrackLooper-actions/blob/cmssw/lowpt_mod.py . Do you know what might be missing?

how is it applied?
in https://github.com/SegmentLinking/TrackLooper-actions/blob/cmssw/cmssw-selfhosted/run.sh#L59C1-L62C1 I see

if [[ "$LOW_PT" == "true" ]]; then
  sed '28r ../lowpt_mod.py' step3_RAW2DIGI_RECO_VALIDATION_DQM.py
fi

but this just dumps the concatenated content of the two files to stdout, as you can see in the log

perhaps cat ../lowpt_mod.py >> step3*.py ?

ariostas · 2026-01-08T17:52:49Z

Oops you're right. I missed the -i

ariostas · 2026-01-08T17:54:14Z

/run gpu-cmssw lowpt

slava77 · 2026-01-08T18:38:45Z

Oops you're right. I missed the -i

btw, why line 28, this seems very brittle: is it the end of the file or do you need to insert before something else?

It would be more practical to add a customise function or even a procModifier in the main cmssw source code for the switch to 0.6 GeV, now that it's a multi-line effort.

ariostas · 2026-01-08T18:43:56Z

Yeah, it's very brittle. It would definitely be ideal to add a procModifier. I haven't done this before, but I'll look into how to do this.

github-actions · 2026-01-08T18:53:13Z

The PR was built and ran successfully with CMSSW (low pT setup) on GPU. Here are some plots.

OOTB All Tracks

The full set of validation and comparison plots can be found here.

ariostas · 2026-01-08T19:14:09Z

/run gpu-cmssw lowpt

github-actions · 2026-01-08T19:22:02Z

There was a problem while building and running with CMSSW on GPU. The logs can be found here.

ariostas · 2026-01-09T17:34:20Z

/run gpu-cmssw lowpt

github-actions · 2026-01-09T18:24:07Z

There was a problem while building and running with CMSSW on GPU. The logs can be found here.

ariostas · 2026-01-13T16:23:29Z

This is now ready for review.

I didn't realize that the low pT setup was broken for CMSSW, so we can't run the comparisons. However, it is now fixed, so it will work after this PR is merged. I did test it locally, and it works. I made standalone comparison plots and they look good. Here are a few of them.

ariostas · 2026-01-13T21:14:02Z

Here's the CMSSW comparison for the low pT setup (0.6 GeV cutoff).

slava77

I'm not sure why after the debugging is complete we need to support reading from the tkLayout csv files.
The cost is some awkward logic that has to pass via the fields defined by the csv files and code that redefines methods that already exist in CMSSW.

Can RecoTracker/LSTCore/interface/LSTGeometry become RecoTracker/LSTGeometry with mostly unconstrained dependencies on CMSSW except for the data format header-only files that are probably useful in the standalone

slava77 · 2026-01-13T18:40:39Z

RecoTracker/LST/plugins/alpaka/LSTModulesDevESProducer.cc

-      setWhatProduced(this, ptCutLabel_);
+        : ESProducer(iConfig), ptCut_(iConfig.getParameter<double>("ptCut")) {


I think we should still have a possibility to have multiple payloads in the same job with different thresholds.

slava77 · 2026-01-13T18:49:20Z

RecoTracker/LSTCore/interface/LSTESData.h

  std::unique_ptr<LSTESData<alpaka_common::DevHost>> loadAndFillESHost(std::string& ptCutLabel);
+  std::unique_ptr<LSTESData<alpaka_common::DevHost>> loadAndFillESHost(lstgeometry::LSTGeometry const& lstg);


perhaps rename; now that we have multiple payloads just having a generic method name is unclear. Also, load feels more relevant for loading from a file.

Suggested change

std::unique_ptr<LSTESData<alpaka_common::DevHost>> loadAndFillESHost(std::string& ptCutLabel);

std::unique_ptr<LSTESData<alpaka_common::DevHost>> loadAndFillESHost(lstgeometry::LSTGeometry const& lstg);

std::unique_ptr<LSTESData<alpaka_common::DevHost>> loadAndFillESDataHost(std::string& ptCutLabel);

std::unique_ptr<LSTESData<alpaka_common::DevHost>> fillESDataHost(lstgeometry::LSTGeometry const& lstg);

slava77 · 2026-01-13T18:55:13Z

RecoTracker/LSTCore/interface/LSTGeometry/LSTGeometry.h

+namespace lstgeometry {
+
+  struct LSTGeometry {


mostly semantics, but still, lst::Geometry is just as clear, a complete repeat lstgeometry::LSTGeometry seems excessive; a global namespace LSTGeometry is also OK

slava77 · 2026-01-13T18:58:32Z

RecoTracker/LSTCore/interface/LSTGeometry/Module.h

+    static unsigned short parseRod(unsigned int);
+    static unsigned short parseRing(unsigned int);
+    static unsigned short parseModule(unsigned int);
+    static unsigned short parseIsLower(unsigned int);


perhaps move to bool

Suggested change

static unsigned short parseIsLower(unsigned int);

static unsigned bool parseIsLower(unsigned int);

slava77 · 2026-01-13T19:00:12Z

RecoTracker/LSTCore/interface/LSTGeometry/Module.h

+    const unsigned short& rod() const;
+    const unsigned short& ring() const;
+    const unsigned short& module() const;
+    const unsigned short& isLower() const;


Suggested change

const unsigned short& isLower() const;

const unsigned bool& isLower() const;

or is there a reason to pretend it can have more values?

slava77 · 2026-01-15T23:12:10Z

RecoTracker/LSTCore/interface/LSTGeometry/CornerMethods.h

+namespace lstgeometry {
+
+  //Calculates the Rodrigues' rotation matrix for rotating a vector around an arbitrary axis.
+  MatrixD3x3 rodriguesRotationMatrix(ColVectorD3 axis, double theta) {


this method is used only in L30, why not inline?

slava77 · 2026-01-15T23:30:01Z

RecoTracker/LSTCore/plugins/LSTGeometryESProducer.cc

+    const DetId detId = det->geographicalId();
+
+    const auto &surface = det->surface();
+    const Bounds *b = &(surface).bounds();


Suggested change

const Bounds *b = &(surface).bounds();

const Bounds *bounds = &(surface).bounds();

slava77 · 2026-01-15T23:33:06Z

RecoTracker/LSTCore/interface/LSTGeometry/CornerMethods.h

+  // Calculates the transformed corners of each sensor
+  void transformSensorCorners(ModuleInfo& moduleInfo) {


isn't all of this simpler to be done with the CMSSW geometry objects (rotations and positions are known; here the code reduces to the minimalist ModuleInfo (probably from parameters used in the tkLayout csv file) to reconstitute what's already known

slava77 · 2026-01-15T23:39:04Z

RecoTracker/LSTCore/interface/LSTGeometry/LSTGeometryMethods.h

+
+    auto detids_etaphi_layer_ref = det_geom.getDetIds([](const auto &x) {
+      auto mod = Module(x.first);
+      return ((mod.subdet() == 5 && mod.isLower() == 1 && mod.layer() != 6) ||


need a comment to describe what this does (exclude the outermost modules that do not have connections to other modules

slava77 · 2026-01-15T23:45:58Z

RecoTracker/LSTCore/interface/LSTGeometry/Module.h

+
+namespace lstgeometry {
+
+  class Module {


I think the bit coding details should just rely on CMSSW equivalent (unless I missed a point that this should remain working in standalone) so that this type is just a struct without parse* methods

ariostas marked this pull request as draft September 26, 2025 15:33

ariostas commented Oct 24, 2025

View reviewed changes

slava77 reviewed Oct 24, 2025

View reviewed changes

slava77 reviewed Oct 25, 2025

View reviewed changes

ariostas force-pushed the ariostas/lst_geometry branch from c2d0f53 to a5accdb Compare November 4, 2025 19:07

ariostas added 19 commits December 3, 2025 19:38

Started implementing code in C++

e36c573

Finished with corners

9c72348

Added IO

0219a91

Format

7bac52c

Added centroid methods

01e5f7d

Added orientation methods

6f5baf4

Started implementing pixel maps

e30647c

Finished with pixel maps

0c561d4

Started implementing module maps

83f4f2e

Some progress on module maps

2477131

Switched namespace to lstgeometry

5577697

A couple of fixes

048ec2b

Finished module maps

650726e

Added standalone binary

f61cbbf

Added a couple of outputs

1a336eb

Write module maps

54eca66

Some progress, but need to switch to gcc13

5a5f071

Write pixel maps

8c9cef1

Fixed pixel maps

52c7348

Tweak cmssw config

1fba827

Switch to numerical parameters

17744c5

ariostas added 2 commits January 13, 2026 14:54

Minor cleanup

e2944b7

Added help option

a5d5f4a

ariostas marked this pull request as ready for review January 13, 2026 16:23

Improve explanation of procedure

777cad3

slava77 reviewed Jan 15, 2026

View reviewed changes

		setWhatProduced(this, ptCutLabel_);
		: ESProducer(iConfig), ptCut_(iConfig.getParameter<double>("ptCut")) {

		std::unique_ptr<LSTESData<alpaka_common::DevHost>> loadAndFillESHost(std::string& ptCutLabel);
		std::unique_ptr<LSTESData<alpaka_common::DevHost>> loadAndFillESHost(lstgeometry::LSTGeometry const& lstg);

	static unsigned short parseIsLower(unsigned int);
	static unsigned bool parseIsLower(unsigned int);

	const unsigned short& isLower() const;
	const unsigned bool& isLower() const;

	const Bounds *b = &(surface).bounds();
	const Bounds *bounds = &(surface).bounds();

		// Calculates the transformed corners of each sensor
		void transformSensorCorners(ModuleInfo& moduleInfo) {

Move LSTGeometry repo into ESProducer and standalone binary #203

Are you sure you want to change the base?

Move LSTGeometry repo into ESProducer and standalone binary #203

Uh oh!

Conversation

ariostas commented Sep 26, 2025

Uh oh!

slava77 commented Sep 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariostas commented Nov 3, 2025

Uh oh!

ariostas commented Dec 3, 2025

Uh oh!

github-actions bot commented Dec 3, 2025

Uh oh!

github-actions bot commented Dec 3, 2025

Uh oh!

ariostas commented Jan 6, 2026

Uh oh!

github-actions bot commented Jan 6, 2026

Uh oh!

github-actions bot commented Jan 6, 2026

Uh oh!

github-actions bot commented Jan 6, 2026

Uh oh!

ariostas commented Jan 6, 2026

Uh oh!

github-actions bot commented Jan 7, 2026

Uh oh!

github-actions bot commented Jan 7, 2026

Uh oh!

github-actions bot commented Jan 7, 2026

Uh oh!

ariostas commented Jan 8, 2026

Uh oh!

github-actions bot commented Jan 8, 2026

Uh oh!

ariostas commented Jan 8, 2026

Uh oh!

slava77 commented Jan 8, 2026

Uh oh!

ariostas commented Jan 8, 2026

Uh oh!

ariostas commented Jan 8, 2026

Uh oh!

slava77 commented Jan 8, 2026

Uh oh!

ariostas commented Jan 8, 2026

Uh oh!

github-actions bot commented Jan 8, 2026

Uh oh!

ariostas commented Jan 8, 2026

Uh oh!

github-actions bot commented Jan 8, 2026

Uh oh!

ariostas commented Jan 9, 2026

Uh oh!

github-actions bot commented Jan 9, 2026

Uh oh!

ariostas commented Jan 13, 2026

Uh oh!

ariostas commented Jan 13, 2026